-
Notifications
You must be signed in to change notification settings - Fork 4.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add signalr args: 'service_mode' and 'allowed_origins' #4716
Add signalr args: 'service_mode' and 'allowed_origins' #4716
Conversation
Fixes hashicorp#4422 by adding service_mode and allowed_origins to the azurerm_signalr_service. Fix formatting Update signalr vendors version Remove old vendor version Move signalr outside of preview folder Fix linting error
f390bfe
to
ead11c3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @Brunhil
Thanks for this PR :)
Taking a look through this is looking good - I've left a few comments inline but if we can fix those up then this otherwise LGTM 👍
Thanks!
- Add error handling when setting features/cors - Alphabetize documentation
@tombuildsstuff - Addressed your comments, thanks for the feedback! I resolved your comments as well, let me know if this is okay to do. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @Brunhil
Thanks for pushing those changes - I've run the tests and it looks like there's a few minor issues to sort - but this otherwise LGTM 👍
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hey @Brunhil
Thanks for pushing those changes - I've run the tests which noticed one issue:
which is failing because the domain "http://www.contoso.com"
isn't a valid CORS Origin (which needs to be a domain name, without the schema). Taking a look through I've left a few other minor comments but if we can fix those up then this otherwise LGTM 👍
Thanks!
hey @Brunhil Thanks for pushing those changes - I've spent a little while debugging this and it appears there's an error in the tests which needed to be fixed to get this to run - as such I hope you don't mind but so that we can get this merged I've pushed a commit which fixes this. Thanks! |
This has been released in version 1.37.0 of the provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading. As an example: provider "azurerm" {
version = "~> 1.37.0"
}
# ... other configuration ... |
I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks! |
Fixes #4422 by adding service_mode and allowed_origins to the azurerm_signalr_service.
Needed to update the signalr api version as the preview version did not have these fields present. Added acceptance tests verifying as well.